Skip to content
This repository was archived by the owner on Apr 14, 2022. It is now read-only.

implement AddImport #1656

Merged
merged 47 commits into from
Oct 29, 2019
Merged

implement AddImport #1656

merged 47 commits into from
Oct 29, 2019

Conversation

heejaechang
Copy link
Contributor

@heejaechang heejaechang commented Oct 9, 2019

Closes #19.

provides add import code action for unknown symbols.

...

  • basic implementation for importing "modules" including submodules + members from loaded modules
  • add tests
  • add add import option [decide not to add option for now]
  • add support for members from unloaded modules
  • improve add import code action ordering a bit
  • open related issue on a relative path (provide refactoring to convert full module name to relative module name in import #1707)
  • provide abbreviation for well-known modules.
  • handle context. ex) specific import based on context (only import type/function and etc)
  • perf ex) cache lint result. run lint only for specific diagnostics and etc - now uses diagnostic service cache
  • show quick fix in hover
  • support reversed abbreviation for well-known modules
  • support unique name on abbreviation

addimport2

currently it does only basic behavior and works only for module known to path resolver and loaded modules (variable module)

no test yet. require many refactorings
the reason it was assert was due to code like below that defines 2 functions with same signature and we only keep 1 symbol out of it.
...
       def f(x):
            class C:
                x = 12
                def m(self):
                    return x
                locals()
            return C

        self.assertEqual(f(1).x, 12)

        def f(x):
            class C:
                y = x
                def m(self):
                    return x
                z = list(locals())
            return C
...
added just one since it is very expensive.
@heejaechang heejaechang changed the title [WIP] implement AddImport implement AddImport Oct 14, 2019
…lready loaded by users code.

for closed files that are never loaded in the editor before, it will use SymbolIndexer to find candidates.

but since the indexer is syntax based, it wont be able to provide information that require semantic info such as imported members

ex) from os import * or __all__.extend(core.__all__)

which means we will not propose

from os.path import join

if os module is not already loaded, instead we will propose ntpath or mac/posixpath since that is where join is actually defined.

unfortunately we can't load modules just for add import, it is too expensive.

we might in future add another index that is based on analyzed data which could have correct info saved. but for now, this is what we decided.
@heejaechang
Copy link
Contributor Author

made not to load modules. can you take another look?

now we show them at the top if there is any.
@heejaechang
Copy link
Contributor Author

@MikhailArkhipov any more feedback?

@Astrantia
Copy link

@heejaechang Will it be possible to add an auto import feature? Importing with a quickfix is great, but it's the best when unresolved imports are auto imported on save.

@jakebailey
Copy link
Member

jakebailey commented Oct 28, 2019

There are so many options for what a particular unresolved name could be that it might be impractical to do it right on save with no user input, or in cases where the unresolved name is just a local error (and not intended to be an import). We're going to start simple and go from there.

(Also note that on-save may be more of a formatting operation, which we don't currently have plans to handle directly.)

@MikhailArkhipov
Copy link

@heejaechang - I'd test it with Anaconda to be sure there are no performance issues and demo it to @Anapo14 to make sure behavior matches spec expectations. Otherwise LGTM.

@heejaechang
Copy link
Contributor Author

thank you!

@heejaechang heejaechang merged commit 254c8f1 into microsoft:master Oct 29, 2019
@grisaitis
Copy link

grisaitis commented Jan 13, 2020

i see that 254c8f1 is not included in any releases, just master. does this mean it's not available in stable vs code?

edit: sorry if this is the wrong place to ask this. i'm pretty confused by the various repos (microsoft/vscode-python, microsoft/python-language-server, etc)

@jakebailey
Copy link
Member

See #19 (comment) (where you also asked this). You can ignore the GitHub releases.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement code action for missing imports
6 participants